Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Android][libraries] Throw PNSE for PersistKeySet flags #57494

Merged
merged 2 commits into from
Aug 17, 2021

Conversation

mdh1418
Copy link
Member

@mdh1418 mdh1418 commented Aug 16, 2021

Fixes #52435

Until support is added for Android with regards to PersistKeySet flags, throw PNSE on Android.

@ghost
Copy link

ghost commented Aug 16, 2021

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #52434

Until support is added for Android with regards to Exportable and PersistKeySet flags, throw PNSE on Android.

Author: mdh1418
Assignees: -
Labels:

area-System.Security

Milestone: 6.0.0

if ((keyStorageFlags & X509KeyStorageFlags.Exportable) == X509KeyStorageFlags.Exportable)
{
throw new PlatformNotSupportedException(SR.Cryptography_X509_PKCS12_ExportableNotSupported);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that the Android implementation currently makes everything exportable. Can someone confirm? (@elinor-fung)

That makes it somewhat odd that the two mobile platforms would have totally opposite behavior on the supported flags and when it throws. I am not feeling good about that and not sure how to proceed. /cc @bartonjs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Android imports are already exportable then the flag can just be ignored. (If non-exportability makes sense then it'd be great if "Exportable wasn't set, ensure non-Exportable" worked now, but I understand the calendar is out of pages).

I actually don't know what state the keys on iOS are in, guess I took it as a given that they weren't exportable.

That makes it somewhat odd that the two mobile platforms would have totally opposite behavior on the supported flags and when it throws.

It's unfortunate, but not unheard of. EphemeralKeySet is preferred for transient operations on Windows and Linux, but throws on macOS. Our Linux stack has no notion of non-exportability, so it's keys are exportable even without Exportable being set. Windows "Exportable" keys aren't always (plain-text) exportable. The only true consistency in this area is how inconsistent things are.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually don't know what state the keys on iOS are in, guess I took it as a given that they weren't exportable.

That's fair question and even though I wrote it I don't have a definitive answer. Technically there are two limitations:

  1. No API to export PKCS#12 (or any other certificate format with private key) exists on mobile Apple platforms
  2. No support for storing private keys in secure enclave on the .NET side

When I started writing the code I simply assumed no export is possible at all due to lack of the API but that's not always true. We reuse the managed PKCS#12 export and hence depend on exporting the raw private keys themselves. That is certainly possible in some circumstances (eg. ephemeral keys) but I never actually verified what happens for persisted keys now. I am not sure if there is some test covering it.

Copy link
Member Author

@mdh1418 mdh1418 Aug 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Android imports are already exportable then the flag can just be ignored.

Will remove this block of code to effectively ignore the flag

@akoeplinger akoeplinger changed the title [Android][libraries] Throw PNSE for Exportable and PersistKeySet flags [Android][libraries] Throw PNSE for PersistKeySet flags Aug 17, 2021
@mdh1418 mdh1418 merged commit b1c6dea into dotnet:main Aug 17, 2021
@mdh1418 mdh1418 deleted the android_add_x509certificate_flag_pnse branch August 17, 2021 18:06
@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Android] X509KeyStorageFlags in X509Certificate constructor are ignored
4 participants